tests/test_hooks.py (1)

78-78: Consider using a lambda for __iter__ to avoid one-shot iterator brittleness.

Mock(return_value=iter([...])) returns the same iterator object on every call. Once exhausted, subsequent iterations yield nothing. If the implementation ever iterates twice, this mock silently returns an empty sequence on the second pass.

Using __iter__ = lambda: iter([...]) creates a fresh iterator per call, matching real manager behavior.

Suggested pattern (applies to lines 78, 101, 135, 167)

-    fake_mgr.__iter__ = Mock(return_value=iter([_make_fake_ext(bad_plugin)]))
+    fake_mgr.__iter__ = lambda: iter([_make_fake_ext(bad_plugin)])

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_hooks.py` at line 78, The test uses Mock(return_value=iter([...]))
for fake_mgr.__iter__ which returns a single, one-shot iterator that becomes
empty after the first iteration; update each occurrence (fake_mgr.__iter__
assignments at the spots using _make_fake_ext, good_plugin/bad_plugin fixtures)
to provide a fresh iterator per call by assigning a callable that returns a new
iterator (e.g., set fake_mgr.__iter__ to a lambda that returns iter([...])) so
iterations behave like a real manager across multiple passes.

tests/test_hooks.py (1)

126-155: Consider adding exception propagation test for run_post_bootstrap_hooks.

The PR objectives mention verifying exception propagation behavior across hook runners. Currently only run_post_build_hooks has an exception propagation test. For complete coverage, consider adding similar tests for run_post_bootstrap_hooks and run_prebuilt_wheel_hooks.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_hooks.py` around lines 126 - 155, Add tests that verify exceptions
raised by hook plugins propagate from run_post_bootstrap_hooks and
run_prebuilt_wheel_hooks the same way as run_post_build_hooks: create a fake
plugin that raises a specific exception, register it via the _get_hooks mock
(similar to test_run_post_bootstrap_hooks_calls_plugin using _make_fake_ext),
call run_post_bootstrap_hooks and run_prebuilt_wheel_hooks with appropriate
minimal args (ctx, req, dist_name, dist_version, sdist_filename/wheel_filename
as needed), and assert the exception is raised (use pytest.raises or
equivalent). Ensure the tests reference the run_post_bootstrap_hooks and
run_prebuilt_wheel_hooks functions and mimic the pattern used in the existing
run_post_build_hooks exception test.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

tests/test_hooks.py (1)

236-259: ⚠️ Potential issue | 🟠 Major

test_log_hooks_logs_each_extension still doesn’t assert emitted log lines.

This test currently validates collaborators only; it does not verify the behavior log_hooks() is meant to provide (per-extension logging), so logging regressions can slip through.

Proposed test hardening

 `@patch`("fromager.hooks.overrides._get_dist_info", return_value=("mypkg", "1.0.0"))
 `@patch`("fromager.hooks.extension.ExtensionManager")
 def test_log_hooks_logs_each_extension(
     mock_em_cls: Mock,
     mock_dist_info: Mock,
+    caplog: pytest.LogCaptureFixture,
 ) -> None:
@@
-    hooks.log_hooks()
+    with caplog.at_level("DEBUG", logger=hooks.logger.name):
+        hooks.log_hooks()
@@
     assert mock_dist_info.call_count == 2
     mock_dist_info.assert_any_call("my_plugins.hooks")
     mock_dist_info.assert_any_call("other_plugins.hooks")
+    messages = [r.getMessage() for r in caplog.records if r.name == hooks.logger.name]
+    assert any("post_build" in m and "my_plugins.hooks" in m for m in messages)
+    assert any("post_bootstrap" in m and "other_plugins.hooks" in m for m in messages)

As per coding guidelines: tests/**: Verify test actually tests the intended behavior. Check for missing edge cases. Flag overly brittle mocks.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_hooks.py` around lines 236 - 259, The test currently only asserts
collaborators were called; update test_log_hooks_logs_each_extension to assert
the actual log output from hooks.log_hooks() by capturing emitted logs (e.g.,
using caplog or a logging.Mock) and verifying that a log entry exists for each
extension (ext_a and ext_b) including their names/module_name; locate the test
setup using mocks ext_a/ext_b and the call to hooks.log_hooks() and after
invoking it assert that the captured logs contain messages referencing
"post_build" / "my_plugins.hooks" and "post_bootstrap" / "other_plugins.hooks"
so the test verifies per-extension logging behavior rather than just call
interactions with mock_em_cls and mock_dist_info.

tests/test_hooks.py (1)

79-95: Assert hook routing in exception-path tests.

These tests prove exception propagation, but they would still pass if a runner called _get_hooks() with the wrong hook name. Add explicit routing assertions.

Suggested hardening

@@ def test_run_post_build_hooks_exception_propagates(mock_get: Mock) -> None:
     with pytest.raises(ValueError, match="hook failed"):
         hooks.run_post_build_hooks(
             ctx=Mock(),
             req=Requirement("pkg"),
             dist_name="pkg",
             dist_version="1.0",
             sdist_filename=pathlib.Path("/tmp/a.tar.gz"),
             wheel_filename=pathlib.Path("/tmp/a.whl"),
         )
+    mock_get.assert_called_once_with("post_build")
@@ def test_run_post_bootstrap_hooks_exception_propagates(mock_get: Mock) -> None:
     with pytest.raises(ValueError, match="hook failed"):
         hooks.run_post_bootstrap_hooks(
             ctx=Mock(),
             req=Requirement("pkg"),
             dist_name="pkg",
             dist_version="1.0",
             sdist_filename=None,
             wheel_filename=None,
         )
+    mock_get.assert_called_once_with("post_bootstrap")
@@ def test_run_prebuilt_wheel_hooks_exception_propagates(mock_get: Mock) -> None:
     with pytest.raises(ValueError, match="hook failed"):
         hooks.run_prebuilt_wheel_hooks(
             ctx=Mock(),
             req=Requirement("pkg"),
             dist_name="pkg",
             dist_version="1.0",
             wheel_filename=pathlib.Path("/tmp/a.whl"),
         )
+    mock_get.assert_called_once_with("prebuilt_wheel")

As per coding guidelines: tests/**: Verify test actually tests the intended behavior. Check for missing edge cases. Flag overly brittle mocks.

Also applies to: 129-145, 177-192

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_hooks.py` around lines 79 - 95, The test
test_run_post_build_hooks_exception_propagates currently only verifies exception
propagation but not that hooks are routed to the correct hook name; update the
test to assert that the patched _get_hooks was called with the specific hook
name used by run_post_build_hooks (e.g.,
mock_get.assert_called_once_with("post_build", any other expected args if
applicable)), and apply the same explicit routing assertion in the other
exception-path tests mentioned (the tests around the other post-build /
pre-publish cases) so each test verifies both exception propagation and that
_get_hooks was invoked for the intended hook identifier.